Skip to content

Conversation

@maki49
Copy link
Collaborator

@maki49 maki49 commented Nov 30, 2024

Fix the problem that XC_LDA_C_PZ overwrites XC_LDA_X in LR-TDDFT.

Si2, lda, gamma (eV) singlet1 singlet2   singlet3 triplet1 triplet2   triplet3
CP2K 2.21*3 2.27*3 3.24 1.31*2 1.4 1.69*3
ABACUS-before 2.616*3 2.704*3 4.21 2.236*3 2.24*3 2.42
ABACUS-fixed 2.17*3 2.29*3 3.37*2 1.34 1.37*2 1.75*3

Copy link
Collaborator

@dyzheng dyzheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes in this PR are relatively simple and clear, with a style consistent with the module_lr module. While there is a difference from the main code style of other modules, this part of the code is relatively independent, and therefore I still recommend accepting it. Suggestions for developers include: 1. memory usage should be as cautious as possible, and manual release should be performed promptly after use. 2. There are multiple occurrences of OMP parallel divisions in the code; since each allocation of OMP tasks requires time, it might be worth considering reducing the number of OMP divisions.

Copy link
Collaborator

@kirk0830 kirk0830 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because files changed in this PR requires the knowledge of libxc, in which many quantities are defined with names commonly seen in libxc, like sigma, ..., So I prone not leave quite many comments on variables' name.
There is one minor suggestion:
I notice you have many simple numerical operations that can be omp-paralleized, a better way to organize your codes can be write several functions like:

void _customize_op1(const std::vector&, ..., ...);

in those functions you implement conditional omp threading, and follow the present coding regulation that do not write for-loop in one-line manner (because human reads line from the left to right, in your code the control of for-loop/bound will be the first to read, which is not the most important information of one for-loop block, instead, the vectorized data being iterated on, and operations are two the most important information. This is the essence of functions like std::transform, std::for_each in aspect of code-readability, as far as I understand) to improve the code readability:
Do not write a for-loop like:

for (int i = 0; i < bound; i++) {/* do something */}

Instead, your write like:

for (int i = 0; i < bound; i++)
{
    /* the thing really important */
}

@kirk0830 kirk0830 removed their assignment Dec 4, 2024
@mohanchen mohanchen added the Refactor Refactor ABACUS codes label Dec 5, 2024
@mohanchen mohanchen merged commit df7e8a5 into deepmodeling:develop Dec 5, 2024
14 checks passed
Fisherd99 pushed a commit to Fisherd99/abacus-BSE that referenced this pull request Mar 31, 2025
* fix the overwriting between xc kernel components

* [pre-commit.ci lite] apply automatic fixes

* use for-loop with omp

* add duration test

* replace lambda by individual function

* expand the for-loop

* small change

---------

Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Refactor Refactor ABACUS codes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants